Overhaul interning. #93148

nnethercote · 2022-01-21T05:44:46Z

A number of types are interned and eq and hash are implemented on
the pointer rather than the contents. But this is not well enforced
within the type system like you might expect.

This PR introduces a new type Interned which encapsulates this concept
more rigorously, and uses it to convert a couple of the less common
interned types.

r? @fee1-dead

rust-highfive · 2022-01-21T05:44:49Z

Some changes occurred in clean/types.rs.

cc @camelid

nnethercote · 2022-01-21T05:50:05Z

@michaelwoerister Ignore the first two commits, they come from #93147, which this PR builds upon. The third commit is the one of interest.

This PR was inspired by (a) my finding the interned types very hard to understand, (b) this GitHub comment, and (c) this Zulip comment.

We could conceivably merge this as is, but it's probably worth converting some of the more heavily used interned types to make sure there aren't any unexpected obstacles. TyS is the holy grail here, but that'll probably happen last :)

@michaelwoerister I'd appreciate feedback about whether you think this is heading in the right direction. Thanks!

lcnr

related to this, i've wrote https://lcnr.de/blog/low-effort-interner/ where I ended up with a type using the same name ^^ using the following type from the "perrrrrrf" section in my blog with edit 2. might be worth considering:

mod private {
    pub struct ProofToken;
}

#[repr(transparent)]
pub struct Uniq<T>(pub T, pub private::ProofToken);

With this, the Eq and PartialEq impls don't need to temporarily use &&T which, while probably easily getting optimized out, might slightly improve perf. More importantly for me, you can now access the T while pattern matching without being able to mutate it, as it will always be behind a immutable reference.

lcnr · 2022-01-21T08:00:18Z

compiler/rustc_middle/src/ty/mod.rs

+    /// will cause incorrect behaviour, though it shouldn't cause undefined
+    /// behaviour. For this reason it is marked `unsafe`.
+    unsafe fn new(ptr: &'tcx T) -> Self {


considering that getting this wrong is memory safe, i would prefer

fn new_unchecked(ptr: &'tcx T) -> Self {

here instead 😆

In my stash I've written these methods, that should cover a lot of use cases, preventing new_unchecked callsites.

pub unsafe fn new_unchecked(inner: &'tcx T) -> Self { Uniq(inner) } pub fn from_arena_interner( value: T, arena: &'tcx WorkerLocal<Arena<'tcx>>, interner: &ShardedHashMap<&'tcx T, ()>, ) -> Self { let inner = interner.intern(value, |v| arena.alloc(v)); Uniq(inner) } pub fn from_make<K, F: FnOnce(K) -> T>( key: K, make: F, arena: &'tcx WorkerLocal<Arena<'tcx>>, interner: &ShardedHashMap<&'tcx T, ()>, ) -> Self { let inner = interner.intern(key, move |key| arena.alloc(make(key))); Uniq(inner) }

michaelwoerister · 2022-01-21T09:23:11Z

I'll reserve some time early next week to take a closer look. I think it's great to try and make the interning infrastructure more robust against accidentally violating invariants!

camelid

Seems like a great idea!

camelid · 2022-01-21T19:01:00Z

src/librustdoc/clean/types.rs

@@ -380,7 +380,7 @@ crate fn rustc_span(def_id: DefId, tcx: TyCtxt<'_>) -> Span {
 }

 impl Item {
-    crate fn stability<'tcx>(&self, tcx: TyCtxt<'tcx>) -> Option<&'tcx Stability> {
+    crate fn stability<'tcx>(&self, tcx: TyCtxt<'tcx>) -> Option<Uniq<'tcx, Stability>> {


FWIW, the function below dereferences the pointer. I didn't realize it was interned, so I thought it was unnecessary to return a reference and had @GuillaumeGomez remove the reference. Maybe convert it back to a reference (with Uniq)?

Sorry, I don't understand this comment. Can you elaborate, or point me at the earlier PR? Thanks.

compiler/rustc_middle/src/ty/mod.rs

camelid · 2022-01-21T19:03:24Z

compiler/rustc_middle/src/ty/context.rs

+
+            impl<'tcx> TyCtxt<'tcx> {
+                pub fn $method(self, v: $ty) -> Uniq<'tcx, $ty> {
+                    let v = self.interners.$name.intern(v, |v| {


Should intern just return Uniq directly?

nnethercote · 2022-01-21T22:07:05Z

Lots of good comments above, this is clearly an idea whose time has come :) I will do more on this next week.

bors · 2022-01-24T03:04:37Z

☔ The latest upstream changes (presumably #93245) made this pull request unmergeable. Please resolve the merge conflicts.

michaelwoerister · 2022-01-24T11:02:47Z

I'd appreciate feedback about whether you think this is heading in the right direction. Thanks!

Yes, this definitely goes in the right direction! I'd even go a step further and try to hide away all the implementation details of interning in a single, generic implementation in rustc_data_structures. Something along the lines of:

Add a new interning module in rustc_data_structures and
- move the InternedSet type to it,
- move ShardedHashMap::intern(), ShardedHashMap::intern_ref(), and ShardedHashMap::contains_pointer_to() to that new InternedSet type.
- move Interned to the module, since it's a private implementation detail of InternedSet,
- remove the rustc_data_structures::sharded::IntoPointer trait, since Interned is the only implementer of it,
- document InternedSet::intern() and InternedSet::intern_ref(): what is make, why don't the methods use value directly? What are the invariants? (+add debug assertions that value and result of make are equal and their hashes too?)
- the make parameter could be renamed to something like move_to_arena or alloc_in_arena.

Uniq should then go into that module too. It's not clear to me if we need the Interned type at all once we have Uniq and a proper InternedSet type. Seemingly, all Interned does is to make sure that we use content hashing and equality instead of pointer hashing and equality. But with Uniq we already have a place where we make that distinction. So InternedSet could look like:

struct InternedSet<'arena, T: Hash + Eq> {
    inner: ShardedHashMap<&'arena T, ()>
}

impl InternedSet<'arena, T: Hash + Eq> {
    pub fn intern<Q>(&self, value: Q, move_to_arena: impl FnOnce(Q) -> &'arena T) -> Uniq<'arena, T> { 
        // ... 
    }
}

It's entirely possible that I'm overlooking some complications here -- but that's roughly what I had in mind for cleaning things up.

michaelwoerister · 2022-01-24T11:07:23Z

We could conceivably merge this as is, but it's probably worth converting some of the more heavily used interned types to make sure there aren't any unexpected obstacles. TyS is the holy grail here, but that'll probably happen last :)

Yes, it sounds like a good idea to add the new interning infrastructure in parallel to the old one and then incrementally move things over, until the old infrastructure can be removed. But trying to move the most complicated case as soon as possible, sounds like a good idea too. After that it should be easy :)

I have a hunch that all of this looks a lot more complicated than it actually is, just because it has grown organically over the years.

nnethercote · 2022-01-25T06:49:20Z

Add a new interning module in rustc_data_structures and

All that makes sense. The latest update doesn't do any of that yet, though. Instead I started again from scratch with TyS, which I got working (tests pass and performance seems fine) though there are still lots of rough edges.

I changed Uniq to be more like how @lcnr did it; Uniq wraps a value, rather than being a smart pointer. Ty<'tcx> is now a &'tcx Uniq<TyS<'tcx>>.

The commits have lots of changes like this:

-impl<'tcx> TyS<'tcx> {
+impl<'tcx> Uniq<TyS<'tcx>> {

and this:

-            hir::CaptureBy::Value if !place.deref_tys().any(ty::TyS::is_ref) => {
+            hir::CaptureBy::Value if !place.deref_tys().any(<Uniq<ty::TyS<'_>>>::is_ref) => {

I.e. all the functionality is moved off TyS as much as possible. These lines are a bit unusual and I don't love it. Ideally they would instead refer to Ty<'tcx> but it's a typedef, not a first-class type, and so can't have its own impl blocks. I've started a further change to make TyS a newtype rather than a typedef, which would improve these things, but that requires a lot more changes and I don't have it working yet.

Anyway, this is where I'm up to. As usual, comments are welcome! Thanks.

nnethercote · 2022-01-25T06:49:32Z

@bors try @rust-timer queue

rust-timer · 2022-01-25T06:49:33Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors · 2022-01-25T06:49:46Z

⌛ Trying commit d75c38ea9a96f53a9eae88b29c335cd28e815f51 with merge 43ea0bf0847986569308356f1623c124b2a46a28...

compiler/rustc_middle/src/ty/util.rs

michaelwoerister · 2022-01-25T11:20:26Z

I changed Uniq to be more like how @lcnr did it; Uniq wraps a value, rather than being a smart pointer.

Interesting! What are the concrete consequences of that?

michaelwoerister · 2022-01-25T13:20:35Z

By the way, a cross cutting change like this will probably need a major change proposal.

nnethercote · 2022-01-25T23:06:24Z

Interesting! What are the concrete consequences of that?

Probably the main one is that Uniq is a good name; with the smart pointer approach it should arguably be called UniqPtr.

Other than that it's not a big difference, I don't think. Switching Ty from a typedef to a newtype is a much bigger change.

bors · 2022-02-15T10:50:44Z

📌 Commit 80632de has been approved by fee1-dead

bors · 2022-02-15T11:59:41Z

⌛ Testing commit 80632de with merge 5569757...

bors · 2022-02-15T14:28:28Z

☀️ Test successful - checks-actions
Approved by: fee1-dead
Pushing 5569757 to master...

rust-timer · 2022-02-15T16:35:14Z

Finished benchmarking commit (5569757): comparison url.

Summary: This benchmark run shows 198 relevant improvements 🎉 but 12 relevant regressions 😿 to instruction counts.

Average relevant regression: 1.0%
Average relevant improvement: -1.1%
Largest improvement in instruction counts: -4.5% on full builds of projection-caching check
Largest regression in instruction counts: 1.8% on full builds of match-stress-enum check

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression

RalfJung · 2022-02-24T15:02:38Z

compiler/rustc_data_structures/src/intern.rs

+/// A reference to a value that is interned, and is known to be unique.
+///
+/// Note that it is possible to have a `T` and a `Interned<T>` that are (or
+/// refer to) equal but different values. But if you have two different


Shouldn't the last word on this line be "equal"? Or I am really confused.

Also "equal but different" sounds rather odd. The key is the location in memory, but that is only first mentioned later.

Maybe:

Note that it is possible to have a T and a Interned<T> to be equal but that are separate values at different addresses.

Yes, that makes more sense. I would probably use &T since that corresponds better with Interned<T> (and talking about addresses only makes sense with references anyway).

Update rustc to 3/6 nightly. The only breaking change comes from rust-lang/rust#93148.

Update rustc to 3/6 nightly. The only breaking change comes from rust-lang/rust#93148, which a warning from rust-lang/cargo#10245.

rust-highfive assigned michaelwoerister Jan 21, 2022

rustbot added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Jan 21, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 21, 2022

nnethercote changed the title ~~Introduce rustc_middle::ty::Uniq.~~ [DRAFT] Introduce rustc_middle::ty::Uniq. Jan 21, 2022

lcnr reviewed Jan 21, 2022

View reviewed changes

camelid reviewed Jan 21, 2022

View reviewed changes

fee1-dead mentioned this pull request Jan 23, 2022

Hash ty::Const using its pointer #90951

Closed

nnethercote force-pushed the Uniq branch from cd30b45 to d75c38e Compare January 25, 2022 06:39

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 25, 2022

This comment has been minimized.

Sign in to view

lcnr reviewed Jan 25, 2022

View reviewed changes

compiler/rustc_middle/src/ty/util.rs Outdated Show resolved Hide resolved

nnethercote force-pushed the Uniq branch from d75c38e to 39f907e Compare January 25, 2022 07:12

lcnr mentioned this pull request Jan 25, 2022

remove TyS::same_type #93290

Merged

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 15, 2022

bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 15, 2022

bors merged commit 5569757 into rust-lang:master Feb 15, 2022

rustbot added this to the 1.60.0 milestone Feb 15, 2022

tmiasko mentioned this pull request Feb 15, 2022

Support pretty printing of invalid constants #94020

Merged

RalfJung reviewed Feb 24, 2022

View reviewed changes

nnethercote deleted the Uniq branch February 25, 2022 03:07

nnethercote mentioned this pull request Mar 4, 2022

Introduce ConstAllocation. #94597

Merged

jdm mentioned this pull request Mar 6, 2022

Update rustc to 3/6 nightly. servo/servo#28732

Merged

bors-servo added a commit to servo/servo that referenced this pull request Mar 6, 2022

Auto merge of #28732 - jdm:rustcup, r=jdm

6294913

Update rustc to 3/6 nightly. The only breaking change comes from rust-lang/rust#93148.

bors-servo added a commit to servo/servo that referenced this pull request Mar 6, 2022

Auto merge of #28732 - jdm:rustcup, r=jdm

ae9fa97

Update rustc to 3/6 nightly. The only breaking change comes from rust-lang/rust#93148, which a warning from rust-lang/cargo#10245.

dario23 added a commit to dario23/rust-semverver that referenced this pull request May 4, 2022

Rustup to rust-lang/rust#93148

ea7321c

dario23 added a commit to dario23/rust-semverver that referenced this pull request May 6, 2022

Rustup to rust-lang/rust#93148

310a4ad

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Overhaul interning. #93148

Overhaul interning. #93148

nnethercote commented Jan 21, 2022 •

edited

Loading

rust-highfive commented Jan 21, 2022

nnethercote commented Jan 21, 2022

lcnr left a comment •

edited

Loading

lcnr Jan 21, 2022

fee1-dead Jan 21, 2022

michaelwoerister commented Jan 21, 2022

camelid left a comment

camelid Jan 21, 2022

nnethercote Jan 24, 2022

camelid Jan 21, 2022

nnethercote commented Jan 21, 2022

bors commented Jan 24, 2022

michaelwoerister commented Jan 24, 2022

michaelwoerister commented Jan 24, 2022

nnethercote commented Jan 25, 2022

nnethercote commented Jan 25, 2022

rust-timer commented Jan 25, 2022

bors commented Jan 25, 2022

This comment has been minimized.

michaelwoerister commented Jan 25, 2022

michaelwoerister commented Jan 25, 2022

nnethercote commented Jan 25, 2022

bors commented Feb 15, 2022

bors commented Feb 15, 2022

bors commented Feb 15, 2022

rust-timer commented Feb 15, 2022

RalfJung Feb 24, 2022

michaelwoerister Feb 24, 2022 •

edited

Loading

RalfJung Feb 24, 2022 •

edited

Loading

Overhaul interning. #93148

Overhaul interning. #93148

Conversation

nnethercote commented Jan 21, 2022 • edited Loading

rust-highfive commented Jan 21, 2022

nnethercote commented Jan 21, 2022

lcnr left a comment • edited Loading

Choose a reason for hiding this comment

lcnr Jan 21, 2022

Choose a reason for hiding this comment

fee1-dead Jan 21, 2022

Choose a reason for hiding this comment

michaelwoerister commented Jan 21, 2022

camelid left a comment

Choose a reason for hiding this comment

camelid Jan 21, 2022

Choose a reason for hiding this comment

nnethercote Jan 24, 2022

Choose a reason for hiding this comment

camelid Jan 21, 2022

Choose a reason for hiding this comment

nnethercote commented Jan 21, 2022

bors commented Jan 24, 2022

michaelwoerister commented Jan 24, 2022

michaelwoerister commented Jan 24, 2022

nnethercote commented Jan 25, 2022

nnethercote commented Jan 25, 2022

rust-timer commented Jan 25, 2022

bors commented Jan 25, 2022

This comment has been minimized.

michaelwoerister commented Jan 25, 2022

michaelwoerister commented Jan 25, 2022

nnethercote commented Jan 25, 2022

bors commented Feb 15, 2022

bors commented Feb 15, 2022

bors commented Feb 15, 2022

rust-timer commented Feb 15, 2022

RalfJung Feb 24, 2022

Choose a reason for hiding this comment

michaelwoerister Feb 24, 2022 • edited Loading

Choose a reason for hiding this comment

RalfJung Feb 24, 2022 • edited Loading

Choose a reason for hiding this comment

nnethercote commented Jan 21, 2022 •

edited

Loading

lcnr left a comment •

edited

Loading

michaelwoerister Feb 24, 2022 •

edited

Loading

RalfJung Feb 24, 2022 •

edited

Loading